Update to #28 #91
Update to #28 #91
Conversation
|
Looks like the test failures are unrelated to this PR, and they existed on master prior to the change. |
in this case |
|
should this be documented somewhere? |
|
Since there are no official docs, I added a section to the README.md |
|
Don't think I have merge rights |
|
Bump @DhairyaLGandhi ? |
DhairyaLGandhi
left a comment
DhairyaLGandhi
left a comment
There was a problem hiding this comment.
Thanks for looking into this. We might be better off using the Module modifier here, but in general I think Main works fine too. We might want to test with using not exported functions/structs from a module to test once
| function _raise_recursive(d::AbstractDict, cache, init) | ||
| if haskey(d, :tag) && haskey(tags, Symbol(d[:tag])) | ||
| cache[d] = tags[Symbol(d[:tag])](applychildren!(x -> raise_recursive(x, cache), d)) | ||
| if Symbol(d[:tag]) in (:ref, :datatype) |
There was a problem hiding this comment.
Can we obviate the need for this check and pass init generically here?
There was a problem hiding this comment.
It would mean every tag[X] needs to be a function of two arguments (d, init) where init is unused for a bunch of cases. Personally, I think the check is cleaner, but I can change it if you like.
There was a problem hiding this comment.
So it might be polluting, hmm.
The init generally seems to be a way to qualify the symbol anyway. I guess one doesn't usually care beyond a type or global ref.
There was a problem hiding this comment.
@DhairyaLGandhi so is this okay to keep? If so, I fixed the other comment about the tests.
4 participants
Fixes #25 using the code in #28. Also added tests.
There was some discussion about a rename and default value. I think for the default, we settled on
init=Mainbeing fine since it wasn't breaking? We can switch to@__MODULE__in a breaking release. What about the rename to the argument? Note that it isn't a kwarg, so the name isn't actually typed out by users.